fix: Weave-Clickhouse - #325
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughClickHouse connections now carry optional replication and cluster-name metadata. v1 conversion resolves these values from environment and structured settings. Managed and external status publication preserves the metadata, with updated CRD schemas and tests. ChangesClickHouse replication topology
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant V1Values
participant Conversion
participant ClickHouseConnection
participant Reconciler
participant ClickHouseStatus
V1Values->>Conversion: Read replication and cluster values
Conversion->>ClickHouseConnection: Set replicated and clusterName
Reconciler->>ClickHouseConnection: Read managed or external connection
Reconciler->>ClickHouseStatus: Publish connection metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/reconciler/clickhouse_replication_test.go (1)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the restating helper comment.
Lines 33-34 only describe what
inferManagedClickHouseStatusdoes. The function name already provides that information.Proposed fix
-// inferManagedClickHouseStatus runs the managed status path and returns the -// published ClickHouse status for the default instance. func inferManagedClickHouseStatus(t *testing.T, replicas int32) apiv2.ClickHouseInfraStatus {As per coding guidelines, “Do not add inline comments that merely restate what code does; comments should concisely explain why.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/reconciler/clickhouse_replication_test.go` around lines 33 - 34, Remove the redundant comment immediately preceding inferManagedClickHouseStatus, leaving the function implementation unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v2/weightsandbiases_types.go`:
- Line 837: Change Replicated from bool to *bool so managed single-replica
ClickHouse preserves false while external ClickHouse leaves it nil; update
managed ClickHouse construction to set the pointer and regenerate deepcopy and
CRD artifacts. Add JSON coverage verifying false, true, and omitted values.
In `@internal/controller/reconciler/clickhouse_replication_test.go`:
- Line 1: Rename the pull request title from “fix/Weave-Clickhouse” to a
Conventional Commit format using an allowed type and an uppercase subject, such
as “fix: Weave ClickHouse”.
- Around line 1-13: Convert the tests in clickhouse_replication_test.go from
testing.T functions to the existing Ginkgo suite configured by suite_test.go,
removing the direct testing import. Register the cases with Ginkgo and replace
testing.T assertions with Gomega matchers, preserving the existing test behavior
and envtest setup.
---
Nitpick comments:
In `@internal/controller/reconciler/clickhouse_replication_test.go`:
- Around line 33-34: Remove the redundant comment immediately preceding
inferManagedClickHouseStatus, leaving the function implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ffcde3b-130a-42cd-971a-4f76192dd742
📒 Files selected for processing (7)
api/v2/weightsandbiases_types.goconfig/crd/bases/apps.wandb.com_weightsandbiases.yamlinternal/controller/infra/managed/clickhouse/altinity/naming.gointernal/controller/infra/managed/clickhouse/altinity/naming_test.gointernal/controller/reconciler/clickhouse.gointernal/controller/reconciler/clickhouse_replication_test.gointernal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml
| type ClickHouseInfraStatus struct { | ||
| WBInfraStatus `json:",inline"` | ||
| Connection ClickHouseConnection `json:"connection,omitempty"` | ||
| Replicated bool `json:"replicated,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(weightsandbiases_types\.go|.*clickhouse.*|.*ClickHouse.*|.*crd.*|.*test.*)$' | head -200
printf '%s\n' '--- relevant symbols and field references ---'
rg -n -C 5 \
'Replicated|managedClickHouseInferStatus|ClickHouseInfraStatus|ClickHouseInfra' \
api config charts deploy test tests 2>/dev/null | head -500 || true
printf '%s\n' '--- type outline ---'
ast-grep outline api/v2/weightsandbiases_types.go --match 'ClickHouseInfraStatus' --view expanded 2>/dev/null || trueRepository: wandb/operator
Length of output: 18184
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all status construction and field assignments ---'
rg -n -C 12 \
'ClickHouseInfraStatus|Replicated\s*[:=]|managedClickHouse|InferStatus|clickhouseStatus' \
api internal config --glob '*.go' --glob '*.yaml' | head -1000
printf '%s\n' '--- CRD schema occurrences ---'
rg -n -C 10 \
'replicated|clusterName|clickhouseStatus|ClickHouseInfraStatus' \
config/crd internal/crdinstaller --glob '*.yaml' --glob '*.go' | head -800
printf '%s\n' '--- likely relevant tests ---'
rg -n -C 15 \
'Replicated|replicated|ClickHouseInfraStatus|clickhouseStatus' \
internal/controller api --glob '*_test.go' | head -1000
printf '%s\n' '--- Make targets and working tree ---'
git status --short
rg -n -C 4 '^(lint|test|manifests|generate|sync-crd-embed):|manifests:|generate:|sync-crd-embed:' Makefile makefile **/Makefile 2>/dev/null | head -400 || trueRepository: wandb/operator
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact Replicated references ---'
rg -n -C 5 '\bReplicated\b|\breplicated\b' --glob '*.go' --glob '*.yaml' . | head -300
printf '%s\n' '--- ClickHouse reconciler symbols ---'
rg -n '^(func|type) .*ClickHouse|ClickHouse.*Infer|clickHouse.*Infer|managedClickHouse|externalClickHouse' internal/controller/reconciler internal/controller/infra --glob '*.go'
printf '%s\n' '--- ClickHouse reconciler focused source ---'
sed -n '1,260p' internal/controller/reconciler/clickhouse.go
printf '%s\n' '--- CRD clickhouseStatus schema ---'
sed -n '4410,4525p' internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml
printf '%s\n' '--- Make targets ---'
rg -n -C 6 '^(lint|test|manifests|generate|sync-crd-embed):|manifests:|generate:|sync-crd-embed:' Makefile */Makefile 2>/dev/null || trueRepository: wandb/operator
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- managed and external status constructors ---'
sed -n '260,325p' internal/controller/reconciler/clickhouse.go
printf '%s\n' '--- replication tests ---'
sed -n '1,155p' internal/controller/reconciler/clickhouse_replication_test.go
printf '%s\n' '--- CRD schema exact subsection ---'
sed -n '4411,4562p' config/crd/bases/apps.wandb.com_weightsandbiases.yaml
printf '%s\n' '--- generated schema consistency ---'
sha256sum \
config/crd/bases/apps.wandb.com_weightsandbiases.yaml \
internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml
cmp -s \
config/crd/bases/apps.wandb.com_weightsandbiases.yaml \
internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml \
&& echo 'schema files identical' || echo 'schema files differ'
printf '%s\n' '--- Makefile targets ---'
rg -n -C 8 '^(lint|test|manifests|generate|sync-crd-embed):|manifests:|generate:|sync-crd-embed:' Makefile */Makefile 2>/dev/null || true
printf '%s\n' '--- tool availability ---'
command -v go || true
command -v make || trueRepository: wandb/operator
Length of output: 17814
🏁 Script executed:
#!/bin/bash
set -eu
cat >/tmp/omitempty_probe.go <<'EOF'
package main
import (
"encoding/json"
"fmt"
)
type BoolStatus struct {
Replicated bool `json:"replicated,omitempty"`
}
type PointerStatus struct {
Replicated *bool `json:"replicated,omitempty"`
}
func main() {
f := false
for _, value := range []any{
BoolStatus{Replicated: false},
BoolStatus{Replicated: true},
PointerStatus{Replicated: nil},
PointerStatus{Replicated: &f},
} {
data, err := json.Marshal(value)
if err != nil {
panic(err)
}
fmt.Println(string(data))
}
}
EOF
if command -v go >/dev/null 2>&1; then
go run /tmp/omitempty_probe.go
else
python3 - <<'PY'
import json
print(json.dumps({}, separators=(",", ":")))
print(json.dumps({"replicated": True}, separators=(",", ":")))
print(json.dumps({}, separators=(",", ":")))
print(json.dumps({"replicated": False}, separators=(",", ":")))
PY
fiRepository: wandb/operator
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- replication defaults and downstream consumers ---'
rg -n -C 8 \
'replicated|Replicated|defaultValue|ReplicatedMergeTree|clickhouse.*status|clickhouseStatus' \
pkg internal hack --glob '*.go' --glob '*.yaml' --glob '*.json' | head -800Repository: wandb/operator
Length of output: 50370
Preserve false for managed single-replica ClickHouse.
bool with omitempty omits the managed false value. Use *bool; set it for managed ClickHouse and leave it nil for external ClickHouse. Regenerate deepcopy and CRD artifacts, and add JSON tests for false, true, and omitted values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v2/weightsandbiases_types.go` at line 837, Change Replicated from bool to
*bool so managed single-replica ClickHouse preserves false while external
ClickHouse leaves it nil; update managed ClickHouse construction to set the
pointer and regenerate deepcopy and CRD artifacts. Add JSON coverage verifying
false, true, and omitted values.
| @@ -0,0 +1,134 @@ | |||
| package reconciler | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a Conventional Commit PR title.
fix/Weave-Clickhouse is not a Conventional Commit title. Rename it, for example, to fix: Weave ClickHouse.
As per coding guidelines, “PR titles and squash commits must use Conventional Commits with an allowed type, and the subject must start with an uppercase letter.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/reconciler/clickhouse_replication_test.go` at line 1,
Rename the pull request title from “fix/Weave-Clickhouse” to a Conventional
Commit format using an allowed type and an uppercase subject, such as “fix:
Weave ClickHouse”.
Source: Coding guidelines
| package reconciler | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| apiv2 "github.com/wandb/operator/api/v2" | ||
| "github.com/wandb/operator/internal/controller/infra/managed/clickhouse/altinity" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/client-go/tools/record" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the configured Ginkgo/Gomega test suite.
This new file uses testing.T tests. Move these cases into the existing Ginkgo suite and use Gomega assertions.
As per coding guidelines, “Use Ginkgo/Gomega for tests; test suites are configured through suite_test.go files and envtest.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/reconciler/clickhouse_replication_test.go` around lines 1
- 13, Convert the tests in clickhouse_replication_test.go from testing.T
functions to the existing Ginkgo suite configured by suite_test.go, removing the
direct testing import. Register the cases with Ginkgo and replace testing.T
assertions with Gomega matchers, preserving the existing test behavior and
envtest setup.
Source: Coding guidelines
HiveMind Sessions1 session · 11m · $2.54
View all sessions in HiveMind → Run |
| // externalClickHouseValues is a minimal v1 external ClickHouse block, so | ||
| // conversion has a connection to attach the replication settings to. | ||
| func externalClickHouseValues(extra map[string]interface{}) map[string]interface{} { | ||
| global := map[string]interface{}{ |
There was a problem hiding this comment.
this is a nit of all nits :), so feel free to ignore for this, but just want to note, we probably at some point want to rewrite all intervace{} to any{} and turn on a linter
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests